Conversation
|
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
r? @adwinwhite rustbot has assigned @adwinwhite. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| new_v1, | ||
| new_v1_formatted, | ||
| next, | ||
| niko, |
This comment has been minimized.
This comment has been minimized.
14ad4f5 to
f35741a
Compare
This comment has been minimized.
This comment has been minimized.
f35741a to
dc753d0
Compare
|
Some of these symbols were added as a perf optimization to avoid having to allocate for identifiers that are commonly used. And pre-interned symbols are represented with less bytes in the crate metadata. |
| BTreeEntry, | ||
| BTreeMap, | ||
| BTreeSet, | ||
| BinaryHeap, |
There was a problem hiding this comment.
These were added in camsteffen@eada4d1 They are still diagnostics items.
There was a problem hiding this comment.
Since then, #138682 gave Clippy the ability to define their own Symbols. Many diagnostic items exist only for Clippy, so I figured only Clippy benefits from the pre-defined Symbols.
| SymbolIntern, | ||
| Sync, | ||
| SyncUnsafeCell, | ||
| T, |
There was a problem hiding this comment.
This is a very common identifier.
There was a problem hiding this comment.
I'd rather try to pre-intern all ASCII letters a-zA-Z, similarly to sym::integer, and see if it makes any difference. Many of them are common.
(Not as a part of this PR.)
There was a problem hiding this comment.
Makes sense. I wonder if there should be a special place for pre-interned symbols that are not sym::_ consts, just to ensure that silly people like me won't spend time auditing them.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hmm, I didn't realize that. I can look for names that are common and add those back. So IIUC we may want to keep I'll wait for the perf run for now. |
But they do reference it. Each crate has an independent symbol interner. There is no cross-crate importing of symbols. |
|
@bjorn3 I'm not familiar with this part and won't have much time looking into it either. Would you like to take over the review since you're already reviewing it? |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (6f9923e): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary 3.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -3.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 483.617s -> 479.802s (-0.79%) |
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
|
@bors r+ |
|
@bors p=2 |
This comment has been minimized.
This comment has been minimized.
Audit Symbols *[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust/pull/152624)* Remove unused symbols and hoist some symbols to Clippy.
|
CI appears stuck (x86_64-gnu-aux specifically) @bors yield |
|
Auto build cancelled. Cancelled workflows: The next pull request likely to be tested is #152624. |
This comment has been minimized.
This comment has been minimized.
Audit Symbols *[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust/pull/152624)* Remove unused symbols and hoist some symbols to Clippy.
|
@bors yield Let's give a rollup a chance first |
|
Auto build cancelled. Cancelled workflows: The next pull request likely to be tested is #152924. |
This comment has been minimized.
This comment has been minimized.
|
The last job is aarch64-apple which tends to finish if you just give it time, so this time I'm not worried about it being stuck :) |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 1c00e98 (parent) -> 3fc3732 (this PR) Test differencesShow 1 test diff1 doctest diff were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 3fc37321c49cb8aed32da426de6145e6d57105da --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (3fc3732): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 1.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -4.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 480.967s -> 482.367s (0.29%) |
View all comments
Remove unused symbols and hoist some symbols to Clippy.